-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Introduce richer EnabledTrait model to handle thread-safe enabled traits computations
#9269
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The enabled traits property was essentially being erased to defaults rather than propagating what was calculated earlier due to how we were returning the DependencyResolutionNode during the pub grub stage. This fix should now assure that we are passing the appropriate enabled traits to these nodes, and are being considered when making calls to PackageContainer's dependency-related methods.
* an EnabledTrait model has been created to store additional data about an enabled trait, namely the origins of its enablement. * EnabledTraits is a wrapper for a Set<EnabledTrait> that handles the special edge cases when dealing with traits. * move traits-related tests in the WorkspaceTests to their own file
* Add new file to PackageModelTests/ for EnabledTrait tests - Tests EnabledTrait, EnabledTraits, and EnabledTraitsMap - Extensions to EnabledTraits added as helpers for testing * Cleanup some TODOs; added some other prints to help debug, will remove once ready for review + tests passing
Contributor
Author
|
@swift-ci please test |
Contributor
Author
|
@swift-ci please test |
Contributor
Author
|
@swift-ci please test |
* Augment EnabledTraits and EnabledTraitsMap to consider the case where a package's default traits are disabled through some parent configuration. * Update methods in Manifest+Traits.swift * Add tests for disable default traits behaviours in EnabledTraitTests.swift and WorkspaceTests+Traits.swift.
Contributor
Author
|
@swift-ci please test |
* For better handling of possible concurrent accesses into the storage of EnabledTraitsMap, since manifests are loaded in parallel, assure that the setter subscript merges the compounded modifications into an atomic action (`mutate`) * Add descriptions to remaining EnabledTraitsMap methods
Contributor
Author
|
@swift-ci please test |
Contributor
Author
|
@swift-ci please test windows |
EnabledTrait model to handle thread-safe enabled traits computations
Closed
1 task
dschaefer2
approved these changes
Nov 5, 2025
Contributor
Author
|
@swift-ci please test windows |
bripeticca
added a commit
to bripeticca/swift-package-manager
that referenced
this pull request
Nov 6, 2025
…read-safe enabled traits computations (swiftlang#9269) This PR introduces the `EnabledTrait` model and associated infrastructure to properly track and manage trait enablement throughout the dependency resolution process. The new system replaces simple string-based trait tracking with a rich model that maintains provenance information about how and why each trait was enabled. Fixes swiftlang#9286 ## Core Changes ### New `EnabledTrait` Model (EnabledTrait.swift) Introduces three main components for tracking enabled traits for packages: **EnabledTrait** - Represents an individual enabled trait with: - Trait name (used as the identifier) - Set of `Setter` values tracking how the trait was enabled (via command-line config, parent package, or another trait) - Support for unifying traits with the same name by merging their setter lists - it's important to note that this is usually done with an `EnabledTrait` for a specific package, and that many packages can define their own (unique) trait that shares a name with a trait from another package but are treated as fundamentally different instances. **EnabledTraits** - Collection wrapper around `IdentifiableSet<EnabledTrait>` that: - Automatically unifies traits with the same name when inserted - Provides convenient set operations (union, intersection) - Maintains enablement metadata while treating traits with the same name as equal (merges `Setters` for traits of the same name) **EnabledTraitsMap** - Thread-safe map storing enabled traits per package that: - Maps `PackageIdentity` to `EnabledTraits`, while maintaining a thread-safe storage box of its properties to accommodate for loading manifests in parallel - Implicitly returns `["default"]` for packages with no explicitly enabled traits - Uses union behaviour: setting traits for the same package multiple times accumulates all traits - Tracks whether default traits have been explicitly disabled by a parent package or trait configuration (set with `[]`). - Tracks parent packages that request `default` trait usage for a package dependency (whether it's been requested through explicitly listing `default` as a trait, or if a trait specification is omitted) The introduction of these models have also provided some API conveniences: **String Interoperability** - `EnabledTrait` and `EnabledTraits` provide seamless string integration: - `ExpressibleByStringLiteral` and `ExpressibleByArrayLiteral` for natural initialization of enabled traits and sets of enabled traits: `let enabledTrait: EnabledTrait = "foo"` or `let enabledTraits: EnabledTraits = ["foo", "bar"]` - Bidirectional string equality: `enabledTrait == "foo"` and `"foo" == enabledTrait` both work - `EnabledTraitConvertible` protocol allows APIs to accept string collections and auto-convert them to `EnabledTrait` **Collection Protocol Support** - `EnabledTraits` conforms to `Collection` with set-like operations (union, intersection, contains) and overloaded methods that accept either `Collection<String>` or `Collection<EnabledTrait>`, enabling flexible APIs that work with both types ## Refactored Trait Handling (`Manifest+Traits.swift`) - Replaced all `Set<String>` trait representations with richer `EnabledTraits` model - Updated validation methods to work with `EnabledTrait` instead of raw strings - Removed parentPackage parameters from validation (now tracked via `EnabledTrait.Setter`) - Improved type safety and metadata tracking throughout trait calculation ### `Workspace+Traits.swift` (new file) - Extracted trait-specific workspace operations - `updateEnabledTraits(for:)` method determines enabled traits for loaded manifests - `updateEnabledTraits(forDependency:_:)` handles dependency-specific trait propagation ## Bug Fixes ### Required Dependencies Calculation **Problem**: During PubGrub dependency resolution, enabled traits were being reset to defaults instead of propagating previously calculated values when creating `DependencyResolutionNode` instances, which relied on an accurate set of `enabledTraits`. **Impact**: Wrong trait sets were considered when evaluating required dependencies, leading to incorrect dependency graphs. **Solution**: Properly propagate enabled traits through the `nodes()` method for `PackageContainerConstraint`, which creates instances of `DependencyResolutionNode` from these constraints during PubGrub dependency resolution. ### IdentifiableSet Intersection **Problem**: The intersection method created an empty set instead of starting with self, causing loss of element data. **Solution**: Changed `var result = Self()` to `var result = self` in `IdentifiableSet.swift:86`, ensuring proper preservation of element data during intersections. - Separated traits tests into dedicated files: - `ModulesGraphTests+Traits.swift` - `WorkspaceTests+Traits.swift` - `EnabledTraitTests.swift` ## Additional Improvements - Enhanced `IdentifiableSet` with `remove(id:)` and `remove(_:)` methods - Added `Workspace+Traits.swift` for trait-specific workspace operations - Updated all package container implementations for proper trait handling
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR introduces the
EnabledTraitmodel and associated infrastructure to properly track and manage trait enablement throughout the dependency resolution process. The new system replaces simple string-based trait tracking with a rich model that maintains provenance information about how and why each trait was enabled.Fixes #9286
Core Changes
New
EnabledTraitModel (EnabledTrait.swift)Introduces three main components for tracking enabled traits for packages:
EnabledTrait - Represents an individual enabled trait with:
Settervalues tracking how the trait was enabled (via command-line config, parent package, or another trait)EnabledTraitfor a specific package, and that many packages can define their own (unique) trait that shares a name with a trait from another package but are treated as fundamentally different instances.EnabledTraits - Collection wrapper around
IdentifiableSet<EnabledTrait>that:Settersfor traits of the same name)EnabledTraitsMap - Thread-safe map storing enabled traits per package that:
PackageIdentitytoEnabledTraits, while maintaining a thread-safe storage box of its properties to accommodate for loading manifests in parallel["default"]for packages with no explicitly enabled traits[]).defaulttrait usage for a package dependency (whether it's been requested through explicitly listingdefaultas a trait, or if a trait specification is omitted)The introduction of these models have also provided some API conveniences:
String Interoperability -
EnabledTraitandEnabledTraitsprovide seamless string integration:ExpressibleByStringLiteralandExpressibleByArrayLiteralfor natural initialization of enabled traits and sets of enabled traits:let enabledTrait: EnabledTrait = "foo"orlet enabledTraits: EnabledTraits = ["foo", "bar"]enabledTrait == "foo"and"foo" == enabledTraitboth workEnabledTraitConvertibleprotocol allows APIs to accept string collections and auto-convert them toEnabledTraitCollection Protocol Support -
EnabledTraitsconforms toCollectionwith set-like operations (union, intersection, contains) and overloaded methods that accept eitherCollection<String>orCollection<EnabledTrait>, enabling flexible APIs that work with both typesRefactored Trait Handling (
Manifest+Traits.swift)Set<String>trait representations with richerEnabledTraitsmodelEnabledTraitinstead of raw stringsEnabledTrait.Setter)Workspace+Traits.swift(new file)updateEnabledTraits(for:)method determines enabled traits for loaded manifestsupdateEnabledTraits(forDependency:_:)handles dependency-specific trait propagationBug Fixes
Required Dependencies Calculation
Problem: During PubGrub dependency resolution, enabled traits were being reset to defaults instead of propagating previously calculated values when creating
DependencyResolutionNodeinstances, which relied on an accurate set ofenabledTraits.Impact: Wrong trait sets were considered when evaluating required dependencies, leading to incorrect dependency graphs.
Solution: Properly propagate enabled traits through the
nodes()method forPackageContainerConstraint, which creates instances ofDependencyResolutionNodefrom these constraints during PubGrub dependency resolution.IdentifiableSet Intersection
Problem: The intersection method created an empty set instead of starting with self, causing loss of element data.
Solution: Changed
var result = Self()tovar result = selfinIdentifiableSet.swift:86, ensuring proper preservation of element data during intersections.Test Improvements
ModulesGraphTests+Traits.swiftWorkspaceTests+Traits.swiftEnabledTraitTests.swiftAdditional Improvements
IdentifiableSetwithremove(id:)andremove(_:)methodsWorkspace+Traits.swiftfor trait-specific workspace operations